-
Notifications
You must be signed in to change notification settings - Fork 144
Update @wordpress/base-styles and replace deprecated variables #4759
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great @samueljseay, thanks for the cleanup.
The code looks good, but 2 comments: One for @jameskoster and the other about removing our usage of core-*
colors.
@jameskoster, this changes the button colors away from Core WP's buttons. Is that the intention for Gutenberg?
Core WordPress button on Ectoplasm theme:
WC-Admin button after this update, also on Ectoplasm (previously it looked just like above):
$dark-gray-600: $core-grey-dark-600; | ||
$dark-gray-700: $core-grey-dark-700; | ||
$dark-gray-800: $core-grey-dark-800; | ||
$dark-gray-900: $core-grey-dark-900; | ||
$gray-900: $core-grey-dark-900; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that these overrides existed before this PR, but I'm having trouble remembering why they would be here. We're just taking colors handed down from Gutenberg and reassigning new colors. Shouldn't we just use $gray-900
instead of our own $core-grey-dark-900
? If I'm understanding correctly, we should replace every instance of $core-grey-dark-900
with $gray-900
in an effort to remove our _colors.scss
.
I think this is what is intended by #4732.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. The only color var we should create is the WooCommerce purple, for the very few cases in which we need to use it.
Edit: Oh, and the colors we use for the charts in Analytics :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I wondered about that one. Most of this was just a find replace. I'll fix this.
The buttons should match core. FWIW they are matching for me on this branch 🤔 I noticed a few small details that are off though. I'm not sure if they're all 100% down to us, or should be addressed in this PR, but I'm going to list them here anyway :)
The purple / blue details in the screenshots above should be mapped to the theme color. |
How can we match other colors from the theme now that base-styles only exposes 3 shades of just one color of the theme? (ie if we want the buttons to be green in the Ectoplasm case) |
For the buttons in particular we should be using the core component out of the box with no overrides, so the theme color should be applied automatically. For our custom components, maybe we can look at core components for implementation inspiration there? I assume they tackled this issue already :D |
@psealock @jameskoster I have tried to be pragmatic about the refactoring of all the grays here, will need some feedback. I found that if I tried to map everything directly to a gray that exists in the @wordpress/base-styles that it dramatically changes the look and feel of screens like analytics and the actions side menu, it makes the entire UI look quite dark and in many places worse if we try to map things as best as possible. I really struggled with this, because I couldn't actually make it look right and with no real baseline to go against I think this approach wasn't going to work. I have opted instead to introduce one more gray variable for WooCommerce. The gray is called $gray-50 and is lighter than any grays in base-styles, but allows us to map most other grays in a better way, it is a blueish light gray shade that existed already in Woo and was used in various places. My thought is that we could look to refactor this gray away later but I think it would require a refactor of the design for those screens like Analytics that heavily rely on grays throughout. 2 relevant commits:
Also I have mapped existing grays:
|
@jameskoster I DM'd you a question about this, but I'll also put it here: Do you mean the Gutenberg button component in @wordpress/components? The reason I ask is, I had a look at the
|
Adding a TLDR of what I said on Slack: We should align our colors with Gutenberg. In this case that means wc-admin buttons with the Ectoplasm theme active should be purple. This is different to wp-admin where they are green, but matches Gutenberg, so that is fine. I noticed these small issues are still outstanding;
Wherever you see the Woo purple, or blue in the screenshots above, let's use the theme color var. Let's try to avoid adding a new custom gray variable, it will only cause headaches down the road. If you can set everything currently using your gray-50 var to gray-100 I am happy to go through with a fine-tooth comb and make adjustments :) |
Opened a PR on this branch here, to remove the |
Everything looks good to me apart from a couple of small things after a quick smoke test and scanning the code changes. |
These were made specifically to correct the look and feel of the inbox and activity card UI.
* Revert "Introduce an interim gray variable till the design can be refactored." This reverts commit cdff94e. * Home screen color updates * Analytics colors
3f11205
to
61ff315
Compare
Fixes #4566 **(and is dependent on the changes from #4759 )** Changes: * Use the new `Card` and `Flex` from `@wordpress/components` * Add contextual tooltips * Adjust existing styles to match [new designs](https://www.figma.com/file/JH9XMFUCOjfXdr3N09AHRD/On-boarding-iterations-June-'20)
Nice job on this one team and especially @samueljseay for seeing this one through |
Fixes #4732
This updates
@wordpress/base-styles
, and because@wordpress/components
depends on variables from it, that must be updated as well.There are 3 major changes required as part of updating:
The $theme-color variable is no longer exposed by base-styles. Instead there are 3 css vars exposed. These are made available by use of a provided mixin. Situations where $theme-color was darkened using scss have been mapped as best as possible to the 2 other darkened shades of the css var that are available such as
--wp-admin-theme-color-darker-20
and--wp-admin-theme-color-darker-10
. In some cases this means the colors are not exactly the same as before.The post css
theme()
call is no longer available. All uses of this have been consolidated to use of the main theme css var--wp-admin-theme-color
. This means that calls liketheme(secondary)
ortheme(outline)
etc have all been consolidated to the one color.Many of the variables used for different shades of gray have been deprecated. These have been mapped across to the new gray variables. (Mapped according to the list described in Update deprecated color vars #4732)
Accessibility
Screenshots
One notable difference is the hover color of calendar days on the date picker. Due to there not being any use of
secondary
any more the color change is more subtle. Gif below:Detailed test instructions:
See the testing instructions from #4558
Theme color settings should match those in the user preferences, but now just shades of the primary color from the theme.
Additional note
I've reached out to Jay to request that he take a look at the approach taken with consolidating colors here. But other reviews are welcome!